Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

snmp: fix dangling pointer in snmp_traps #51

Merged
merged 3 commits into from
Nov 29, 2024
Merged

snmp: fix dangling pointer in snmp_traps #51

merged 3 commits into from
Nov 29, 2024

Conversation

j123b567
Copy link
Contributor

@j123b567 j123b567 commented Nov 1, 2024 •
edited
Loading

fixes: https://savannah.nongnu.org/bugs/?64685
fixes: https://savannah.nongnu.org/bugs/?64847
replaces: #42 which is wrong
replaces: #41 which is duplicate of #42

@j123b567 j123b567 mentioned this pull request Nov 1, 2024
Closed
@yarrick
Copy link
Member

yarrick commented Nov 26, 2024 •
edited
Loading

With this contrib/ports/unix/example_app still fails to compile on my machine (using GCC 14.1):

cc -g -DLWIP_DEBUG -Wall -pedantic -Werror -Wparentheses -Wsequence-point -Wswitch-default -Wextra -Wundef -Wshadow -Wpointer-arith -Wcast-qual -Wc++-compat -Wwrite-strings -Wold-style-definition -Wcast-align -Wmissing-prototypes -Wnested-externs -Wunreachable-code -Wuninitialized -Wmissing-prototypes -Wredundant-decls -Waggregate-return -Wlogical-not-parentheses -Wlogical-op -Wc90-c99-compat -Wtrampolines -I. -I../../.. -I../../../../src/include -I../../../ports/unix/port/include -I../../../examples/example_app  -c ../../../../src/apps/snmp/snmp_traps.c
../../../../src/apps/snmp/snmp_traps.c: In function ‘snmp_send_trap_or_notification_or_inform_generic’:
../../../../src/apps/snmp/snmp_traps.c:401:24: error: storing the address of local variable ‘snmp_v2_special_varbinds’ in ‘*varbinds.prev’ [-Werror=dangling-pointer=]
  401 |         varbinds->prev = &snmp_v2_special_varbinds[1];
      |         ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../../src/apps/snmp/snmp_traps.c:357:23: note: ‘snmp_v2_special_varbinds’ declared here
  357 |   struct snmp_varbind snmp_v2_special_varbinds[] = {
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~
../../../../src/apps/snmp/snmp_traps.c:347:176: note: ‘varbinds’ declared here
  347 | uct snmp_msg_trap *trap_msg, const struct snmp_obj_id *eoid, s32_t generic_trap, s32_t specific_trap, struct snmp_varbind *varbinds)
      |                                                                                                       ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~

cc1: all warnings being treated as errors

@j123b567
snmp: remove handling of varbind->prev because it is never used
55e4e2d 
No code in snmp traverses varbinds backwards so prev is never used.
This also fixes false positive dangling pointer detection by GCC.
@j123b567
Copy link
Contributor Author

This function is a trap :(

This is not a real dangling pointer, because it is fixed afterwards, but the compiler is not smart enough to see it.

original_varbinds->prev = original_prev;

And I understand, because the code is terrible.

There is a much bigger problem with the SNMP code. snmp_varbind is defined with prev but it is never used in any part of SNMP. Only the snmp_trap code tries to set it correctly, but all traversal functions use only next.

Is it ok for you to remove all prev handling to prevent this situation?

This function had two dangling pointers. One was real and is fixed by the first commit, the second was false positive and is fixed by the second commit.

yarrick
yarrick requested changes Nov 27, 2024
View reviewed changes
Copy link
Member

@yarrick yarrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove the prev pointer in struct snmp_varbind also to make sure it isn't used?

@j123b567
Copy link
Contributor Author

Sure.

I thing, that it is breaking change for user applications. Nobody is probably using prev field but everybody is setting it to NULL during initialization of snmp_varbind.

@j123b567 j123b567 requested a review from yarrick November 29, 2024 10:57
@j123b567
Copy link
Contributor Author

Should I also add something like this to UPGRADING?

  * The prev field in the snmp_varbind structure has been removed.

@yarrick
Copy link
Member

yarrick commented Nov 29, 2024

I can add it for you.

@yarrick yarrick merged commit 5ee5801 into lwip-tcpip:master Nov 29, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yarrick yarrick yarrick approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@j123b567 @yarrick